Repository navigation
feat: support geospatial results on the kernel path - #990
Conversation
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Medium
Solid, well-tested feature (kernel geospatial passthrough + thrift description logical-type derivation). One medium concern: the default-True option hard-fails every kernel session against a wheel that lacks the kwarg, while the pip constraint (^1.1.0) still permits such wheels — worth gating the default case or bumping the minimum in lockstep. The thrift positional-field switch and the variant/geometry/geography regex both look correct and are covered by new tests.
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Medium
Solid, well-tested change wiring a kernel-only enable_geospatial_support option and generalizing the Thrift logical-type extraction (variant → variant/geometry/geography, positional field lookup to handle duplicate aliases). One medium documentation mismatch: CONNECTION_PARAMETERS.md lists the default as True while code, docstring, CHANGELOG, and tests all default it to False. The intentional always-explicit _kernel_geospatial_kwargs (which hard-fails old kernel wheels even at the default) is clearly deliberate and covered by tests.
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Medium
Solid, well-tested feature. One medium concern worth maintainer confirmation: the default-False geospatial path raises NotSupportedError on older kernel wheels, unlike every other optional kwarg which degrades gracefully — so a connector upgrade hard-breaks all kernel connections against an older wheel even for users who never enable geospatial. The thrift _col_to_description regex and positional-index schema-lookup changes look correct and are well-covered by the new tests.
Align Kernel E2E with case-sensitive filtering from databricks-sql-kernel#393. Check uppercase matches and lowercase/mixed-case empty results, and update stale filter comments. Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 High
One high-severity backward-compat concern: the enable_geospatial_support kwarg is sent to the kernel unconditionally and hard-fails against any ^1.1.0-permitted kernel wheel lacking it, even at the default False — breaking existing default kernel connections after a connector-only upgrade. The Thrift _col_to_description regex, positional _hive_schema_to_description lookup, and the result-set/description tests otherwise look correct and well-covered.
Refresh status contexts after GitHub reported the Kernel E2E requirement as expected despite matching successful checks. The source tree is unchanged from the validated fix in 3c800a5. Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Medium · 1 Low
Solid, well-tested feature addition. One medium concern: the geospatial kwarg hard-fails on default-disabled connections against older/currently-published kernel wheels (release must be gated on the #991 floor bump, or the default case should fall closed like max_connections). Plus a low note on the new positional Arrow-field mapping assuming schema order parity for all Thrift descriptions.
Summary
enable_geospatial_supportconnection option{"srid": int, "wkb": bytes}values when enabled and WKT / EWKT strings when disabledDependency
Pins kernel
9c2e2e6182fc7285bcfff39b2177941bc804897c, including kernel #361. Before a connector release, #991 must land to raise the optional published-kernel dependency floor from^1.1.0to^1.2.0.Validation
Merge-fix validation
3c800a507b23817cc764aa7c87964dda5a5e71e1.test_tables_table_types_filter_is_case_sensitive; proxy traffic verification passed.